Add admin API token fallback for watcher notifications#1298
Add admin API token fallback for watcher notifications#1298nevyangelova wants to merge 2 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughcheckIssueWatchers now treats connected-user fetch errors as terminal; if no connected Jira client is available and AdminAPIToken is configured, it falls back to fetching issue watchers via a new exported GetWatchersWithAPIToken HTTP call to the Jira REST API. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant checkIssueWatchers
participant ConnectedUserClient
participant AdminTokenClient
participant JiraAPI
Client->>checkIssueWatchers: trigger watcher check
checkIssueWatchers->>ConnectedUserClient: fetch connected user client
alt fetchConnectedUser returns error
checkIssueWatchers-->>Client: return (terminal error)
else connected client != nil
checkIssueWatchers->>ConnectedUserClient: GetWatchers(issueID)
ConnectedUserClient->>JiraAPI: GET /rest/api/2/issue/{issueID}/watchers (user auth)
JiraAPI-->>ConnectedUserClient: 200 + watchers JSON
ConnectedUserClient-->>checkIssueWatchers: watchers
else connected client is nil and AdminAPIToken set
checkIssueWatchers->>AdminTokenClient: GetWatchersWithAPIToken(issueID, instanceID)
AdminTokenClient->>JiraAPI: GET /rest/api/2/issue/{issueID}/watchers (admin token)
JiraAPI-->>AdminTokenClient: 200 + watchers JSON
AdminTokenClient->>AdminTokenClient: validate + unmarshal JSON
AdminTokenClient-->>checkIssueWatchers: jira.Watches
end
checkIssueWatchers-->>Client: complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
This pull request introduces a critical potential Server-Side Request Forgery (SSRF) in server/issue.go: GetWatchersWithAPIToken constructs and requests a URL using an unvalidated instanceID parameter, allowing an attacker-controlled value (including internal IPs) to trigger arbitrary HTTP requests. The finding is rated critical but non-blocking and should be fixed by validating/restricting instanceID, enforcing allowed hosts or schemes, and using safe HTTP client patterns.
🔴 Potential Server-Side Request Forgery (SSRF) in
|
| Vulnerability | Potential Server-Side Request Forgery (SSRF) |
|---|---|
| Description | GetWatchersWithAPIToken constructs a URL using the instanceID parameter as the base URL and performs an HTTP request (httpClient.Do) without validating or restricting the instanceID value. If instanceID is attacker-controllable and contains an arbitrary URL (including internal IPs), this allows SSRF. |
mattermost-plugin-jira/server/issue.go
Lines 1913 to 1916 in a0ade1c
Comment to provide feedback on these findings.
Report false positive: @dryrunsecurity fp [FINDING ID] [FEEDBACK]
Report low-impact: @dryrunsecurity nit [FINDING ID] [FEEDBACK]
Example: @dryrunsecurity fp drs_90eda195 This code is not user-facing
All finding details can be found in the DryRun Security Dashboard.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/issue.go`:
- Around line 1684-1694: The current branch treats a non-nil err from the
connected-user lookup the same as a nil client (no linked user) and falls back
to the admin token; change the logic so that if err != nil you log and return
immediately (using p.errorf and the existing error message), and only when err
== nil && client == nil (i.e., no linked Jira user) should you attempt the
admin-token fallback via p.GetWatchersWithAPIToken; keep references to
GetWatchersWithAPIToken, p.getConfig().AdminAPIToken, and p.errorf to locate and
update the branch accordingly.
- Around line 1907-1919: The http.Client used in the watcher fetch is created
without a timeout which can hang; update the code in GetIssueWatchers (and
similarly in GetIssueDataWithAPIToken and GetProjectListWithAPIToken) to
construct an http.Client with a sensible Timeout (e.g., time.Second * X) before
calling http.NewRequest/Do, and reuse that client variable (httpClient) for the
request; ensure you import time if not present and preserve existing error
handling (SetAdminAPITokenRequestHeader, errors.Wrapf) while setting the timeout
on the client prior to httpClient.Do.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5b04062e-10db-4a05-b6d1-71d7370808b8
📒 Files selected for processing (1)
server/issue.go
…ent timeout - Only fall back to admin API token when no connected user is found (client == nil), not on hard errors like instance store failures - Add 30s timeout to HTTP client in GetWatchersWithAPIToken to prevent indefinite blocking on unresponsive Jira servers Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/issue.go`:
- Around line 1911-1915: The request URL in GetWatchersWithAPIToken currently
uses the store key parameter instanceID instead of the Jira base URL; update the
function to resolve the instance object (via the existing LoadInstance or
equivalent on Plugin) using the instanceID key and use the instance's
BaseURL/URL (e.g., instance.BaseURL or instance.URL) when formatting the request
to "/rest/api/2/issue/{issueID}/watchers"; ensure you still handle LoadInstance
errors and preserve the existing error wrapping and http client usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f7e43f74-f92b-43b9-ac05-d6cc380303bc
📒 Files selected for processing (1)
server/issue.go
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
avasconcelos114
left a comment
avasconcelos114
left a comment
There was a problem hiding this comment.
The changes LGTM! It might also be good to cover this new fallback scenario with test cases in issue_test.go (I'd consider it non-blocking though)
3 participants
Summary
When none of the issue's Creator, Assignee, or Reporter have connected Mattermost accounts, watcher notifications silently fail because the plugin cannot obtain a Jira API client to fetch the watcher list. This adds a fallback to use the configured Admin API Token to retrieve watchers, matching the existing fallback pattern used for issue data and project list fetching.
Ticket Link
https://mattermost.atlassian.net/browse/MM-68049
Change Impact: 🟠 Medium
Reasoning: The PR adds a fallback mechanism to retrieve issue watchers using an admin API token when no connected user client is available, following existing patterns in the codebase. However, the new
GetWatchersWithAPITokenmethod and its integration into thecheckIssueWatchersflow lack dedicated test coverage for the token-based fallback paths.Regression Risk: Low to moderate. The change follows established fallback patterns already used for
GetIssueDataandGetProjectListData. Error handling is explicit—hard errors fromfetchConnectedUserreturn immediately, while the fallback only triggers whenclient == nil. The token-based HTTP call uses a 30-second timeout to prevent indefinite blocking. However, the new code paths (token-based watcher retrieval) have not been explicitly tested, creating untested notification flow scenarios.QA Recommendation: Manual QA should focus on watcher notification scenarios where no connected user exists but an admin token is configured. Test both success and failure cases: (1) valid admin token retrieving watchers successfully, (2) missing/invalid token, (3) unresponsive Jira server (timeout handling), and (4) malformed responses. Automated test coverage for the new
GetWatchersWithAPITokenmethod and its integration paths should be added before merging. Risk of skipping manual QA is moderate due to the gap in test coverage for token-based fallback flows.